-
Notifications
You must be signed in to change notification settings - Fork 3k
Use dev services instead of docker-maven-plugin for Postgresql #51066
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Use dev services instead of docker-maven-plugin for Postgresql #51066
Conversation
|
/cc @gsmet (hibernate-orm) |
This comment has been minimized.
This comment has been minimized.
|
Yes, it's a trick in the source itself :) Originally, the JDBC Extension queried the JDBC URL value at build time to either start or skip the DevService. In cases where the URL was an expansion, this would fail if the expansion is not available at build time. In the original issue, the JDBC URL was set as an expansion in Vault, only available at runtime (see what I mean that we shouldn't be querying runtime configuration at build time? :)) So I've changed that to just query for the name to be available, since we don't care about the value at all when we have to determine if we launch the DevService. To simulate the original issue, the source only sets Lines 20 to 39 in 1243412
So in this case, you get a value at build time, which disables the DevService, and then when runtime starts, there is no value, which causes the message saying that the URL is not set. Testing for this particular situation when using DevServices is mutually exclusive. We can either remove the config source and trust that our code is not querying for the actual config value (by always using |
Thank you for the explanation! So this would be a correct refactoring of the original test? Without this PR, that is. From to |
|
Yes, it doesn't really matter what the expression configuration name is. We just want to ensure that the expression resolution is not available at build time and that we don't try to expand it, as that would cause an error. |
1243412 to
caeda87
Compare
Got it, thanks! I'll switch the property name to make it more clear it's not supposed to work. Since the other property was also used in the application.properties I thought it was supposed to be resolvable. I've reworked the tests and I have something that passes, and I think it still exercises the same things, but I'd value a check from @radcortez. What I've done is use dev services for most of the tests in that project, and added a new test which uses a resource lifecycle manager to start postgres. I also tried |
This comment has been minimized.
This comment has been minimized.
caeda87 to
cf5c0e8
Compare
Status for workflow
|
|
Excellent! |
|
|
||
| ``` | ||
| mvn clean install -Dtest-containers | ||
| mvn clean install -Dtest-containers -Dpostgres.url=jdbc:postgresql://... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed this in your other PRs... but I think it's incorrect? postgres.url is no longer mentioned in the configuration of this integration test, so I don't think setting it will do anything.
In order for this to be true, I think you'd need to keep the line quarkus.datasource.jdbc.url=${postgres.url} in application.properties. And set the default value of postgres.url to an empty string in pom.xml, so that by default the configuration will look like quarkus.datasource.jdbc.url= (no value), which means dev services will kick in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That, or we remove this part of the README and expect people to always use dev services to run these tests. Which is perfectly fine IMO, but others might disagree -- @gsmet maybe?
| <!-- <dependency>--> | ||
| <!-- <groupId>org.junit.platform</groupId>--> | ||
| <!-- <artifactId>junit-platform-suite</artifactId>--> | ||
| <!-- <scope>test</scope>--> | ||
| <!-- </dependency>--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be removed? Or maybe we need a comment to explain why it's here and commented out?
Partial resolution of #44124.
As with #51059, I left the
extensions/*project using an independently-managed container, since the test exercisesapplication.propertiesa lot.I'm marking this as a draft, because I had a failure I can't explain. I'm hoping @radcortez and @yrodiere might be able to help. I don't know if it's a bug, or if I'm asking the test to do something that's not supported. The
jpa-postgresqlproject fails withThe reason seems to be related to the config override,
OverrideJdbcUrlBuildTimeConfigSource, which sets aquarkus.datasource.jdbc.url, to reproduce #16123. If I disable that override by deletingsrc/test/resources/META-INF/services/org.eclipse.microprofile.config.spi.ConfigSource, the test all passes. With the override, I'd expect a connection failure, becausequarkus.datasource.jdbc.urlwould disable the dev service, and there's nothing listening at the hardcoded endpoint. But instead I get an error saying thequarkus.datasource.jdbc.urlproperty isn't set. It's like the dev service processor sees it as set, so doesn't start anything, but theAgroalRecordersees it as not set. Maybe that's expected because of when the reading and writing of the config happens, but it makes me wonder if the test is testing what it's supposed to be testing. As far as I can tell, the override was just setting the same value as was set inapplication.propertiesalready.